Force OPf_PARENS on "if/elsif/unless" optree branches #23850
Conversation
Previously, only `else {}` branches would have the OPf_PARAMS flag set.
`Perl_op_scope` uses this flag to determine whether its optree argument (`o`)
should be wrapped in an `ENTER/LEAVE` pair or only get a `SCOPE` OP, which
is typically optimized away (nulled out) before runtime.
This has at least two consequences visible for Perl users:
1. Differing lifetimes for things depending upon whether they occur in an
`if` block or an `else` block. This could cause bugs that cannot be
understood from Perl source code alone.
For example, consider a `Foo` class that has a `DESTROY` sub. In the
following code, `$object2` goes out of scope at the completion of the
`else {}` block and the `DESTROY` sub fires. In contrast, `$object1`
does NOT go out of scope at the completion of the `if {}` block -
because _there is no scope_ - and the `DESTROY` sub won't fire until
some later time.
```
if ($_) {
my $object1 = Foo->new();
} else {
my $object2 = Foo->new();
}
```
2. The `NEXTSTATE` OP immediately following a `SCOPE` OP is typically
nulled out before runtime, but the first `NEXTSTATE` after an
`ENTER` OP is not.
`NEXTSTATE` OPs update the interpreter with the line number associated
with the currently executing statement. (`PL_curcop`.) The interpreter
outputs this in warnings or fatal error messages. Not having the first
`NEXTSTATE` present in `if` blocks means that error messages triggered
by the first line of code will typically report an incorrect line
number.
This commit addresses the above two concerns, but with the downside
that `if`/`elsif`/`unless` blocks now have the same OP overhead as
`else` blocks. (The `ENTER`, first `NEXTSTATE`, and `LEAVE` OPs.)
|
Line number problems could also be fixed by not nulling out Something that we could do if this PR is merged and we fix the above is to teach
|
It might be easier than I suggested above. 😆 Will look into that sometime this month. |
|
Hmmm, a better fix for the That does nothing for the line number warnings though - actually means lines from the start of |
|
"Force OPf_PARAMS..." in subject, commit message. but you force |
D'oh. Too much thinking about parameters recently. Thanks for spotting it. I'm going to see if the alternative route works without breaking B, before continuing with this PR. |
|
I like fixes, but I am worried it will silently change lifetimes and break downstream code. Though it should really only depend on destruction side effects, code that depends on the object really needs its own reference will be prevent any early destruction. |
Yeah, that's definitely a potential downside. Any such code is already brittle; any minor refactoring that moves logic out of an |
richardleach
changed the title
|
I'm closing this in favour of #23867, which I think is the better approach. |
2 participants
Previously, only
else {}branches would have the OPf_PARAMS flag set.Perl_op_scopeuses this flag to determine whether its optree argument (o)should be wrapped in an
ENTER/LEAVEpair or only get aSCOPEOP, whichis typically optimized away (nulled out) before runtime.
This has at least two consequences visible for Perl users:
Differing lifetimes for things depending upon whether they occur in an
ifblock or anelseblock. This could cause bugs that cannot beunderstood from Perl source code alone.
For example, consider a
Fooclass that has aDESTROYsub. In thefollowing code,
$object2goes out of scope at the completion of theelse {}block and theDESTROYsub fires. In contrast,$object1does NOT go out of scope at the completion of the
if {}block -because there is no scope - and the
DESTROYsub won't fire untilsome later time.
The
NEXTSTATEOP immediately following aSCOPEOP is typicallynulled out before runtime, but the first
NEXTSTATEafter anENTEROP is not.NEXTSTATEOPs update the interpreter with the line number associatedwith the currently executing statement. (
PL_curcop.) The interpreteroutputs this in warnings or fatal error messages. Not having the first
NEXTSTATEpresent inifblocks means that error messages triggeredby the first line of code will typically report an incorrect line
number.
This PR addresses the above two concerns, but with the downside
that
if/elsif/unlessblocks now have the same OP overhead aselseblocks. (TheENTER, firstNEXTSTATE, andLEAVEOPs.)